Number input#23842
Conversation
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
changes to some of the other widgets.
|
range and percision should prob be added but im fine with that coming in a later PR |
jordanhalase
left a comment
There was a problem hiding this comment.
My thoughts on this
kfc35
left a comment
There was a problem hiding this comment.
Reviewed everything. The example seems to work fine for me. Just have a few comments/questions
| { | ||
| drag_end.propagate(false); | ||
| if state.0 && !disabled { | ||
| let local_pos = transform.try_inverse().unwrap().transform_point2( |
There was a problem hiding this comment.
nit: I was looking if some of this logic has been extracted into a function (calculating a normalized local position), and there is a function on ComputedNode that does parts of this logic: normalize_point
Just for your consideration. I’ve noticed similar logic used in color_plane
| /// Component which causes the color of a text span to be set based on a theme color. Unlike | ||
| /// [`InheritableThemeTextColor`], this can work when set directly on the text span entity, and is | ||
| /// not inherited. | ||
| // TODO: This is necessary because an entity with Propagate doesn't update itself, only its |
There was a problem hiding this comment.
What is the action item in this TODO?
There was a problem hiding this comment.
The real action item here is not to use Propagate, but something else. However, that's a much larger discussion.
| } | ||
|
|
||
| /// Observer function which updates the [`NumberInputValue`] in response to a [`ValueChange`] event. | ||
| pub fn number_input_self_update(value_change: On<ValueChange<f32>>, mut commands: Commands) { |
There was a problem hiding this comment.
I’m not sure if this is a plausible scenario that merits fixing, but If focus is not on the EditableText, this particular observer is hooked in, and NumberInputValue is inserted by some other system, does this make a loop?
NumberInputValueis insertednumber_input_on_value_changeis triggeredOn<Insert, NumberInputValue>- Because
EditableTextdoes not have focus, it queues editable_text edits apply_text_editsemits aTextEditChangenumber_input_on_text_changeis triggeredOn<TextEditChange>number_input_on_text_changeemits aValueChangenumber_input_self_updateis triggeredOn<ValueChange<f32>- A
NumberInputValueis inserted
I assume because it requires that focus not be on the EditableText that this scenario probably won’t happen, but figured I’d bring it up if it’s actually important or to clear up if I’m misunderstanding something
There was a problem hiding this comment.
We could avoid this by changing the self update function to only update the value if the value has changed. This would break the cycle.
Updating the number input is now done by triggering an event rather than inserting a component.
| drag_end.propagate(false); | ||
| commands.entity(slider_ent).remove::<Pressed>(); | ||
| if drag.dragging { | ||
| if drag.dragging && !disabled { |
There was a problem hiding this comment.
What happens if InteractionDisabled is added middrag, won't it get stuck with Pressed and drag.dragging = true.
Same with the others, shouldn't we remove Pressed from a control on release, even if it has InteractionDisabled
There was a problem hiding this comment.
That's a good idea.
| /// Used to indicate what format of numbers we are editing. This primarily affects the type | ||
| /// of [`ValueChange`] event that is emitted. | ||
| #[derive(Component, Default, Clone, Copy)] | ||
| pub enum NumberFormat { |
There was a problem hiding this comment.
I was thinking of a value input design with an InputValuePlugin<T> for each supported value type T, rather than an enum discriminator. So users could have an input for any type that can be parsed from a string. Then it could support a Val input for instance where values like "10px" or "25%" are both valid, for example.
There was a problem hiding this comment.
I'm not against merging this, and leaving discussions about generalization until later though.
There was a problem hiding this comment.
I prefer that design too :)
kfc35
left a comment
There was a problem hiding this comment.
I think this is in a good enough state to merge and to have some follow ups later for anything still pending (except for my comment about drag end and InteractionDisabled).
| :group_body | ||
| Children [ | ||
| :label("A standard group"), | ||
| :label_small("Scalar property"), |
There was a problem hiding this comment.
When running the example, there’s a bit of unexpected behavior here now:
- Focus on the first scalar property
- Update the value to something different
- Tab navigate to the second scalar property (copy)
Note that, here, the second scalar property is not updated with the value from the first scalar property field - Tab navigate again away from the second scalar property (or just click anything but the first scalar property to unfocus the second scalar property)
Observe that the first scalar property is set back to the value that is in the second scalar copy property
I expected the first scalar property’s change to be reflected in the second scalar property after I tab switched focus from the first to the second scalar copy.
I suspect the cause is due to number_input_on_update not applying UpdateNumberInput to focused entities.
However, I don’t feel this is too pressing to fix in this PR if it’s not trivial to fix / would affect other actually desirable UX. This can be a follow up if this is something that should be fixed. This is a bit of a contrived example and I don’t know how much this will actually be surfaced in practice.
There was a problem hiding this comment.
What you are seeing is a race condition between the two focus events: because the second input acquires focus before the focus-lost event is received for the first input, this blocks the second one from being updated.
alice-i-cecile
left a comment
There was a problem hiding this comment.
I'm happy enough with this design now; I'd prefer to merge and iterate :)
Objective
Part of #19236
Solution
A numeric text input widget. This includes:
Not supported:
Additional changes in this PR:
is_finalflag inValueChange. This is necessary to allow listeners the choice between spammy and not-spammy updates when listening to widget outputs.Testing
Manual testing
Showcase